Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken links in Payment API #31526

Merged
merged 21 commits into from
Jan 22, 2024
Merged

Fix broken links in Payment API #31526

merged 21 commits into from
Jan 22, 2024

Conversation

skyclouds2001
Copy link
Contributor

@skyclouds2001 skyclouds2001 commented Jan 5, 2024

Description

updateWith() method is defined on PaymentRequestUpdateEvent and PaymentMethodChangeEvent simply inherit this method from PaymentRequestUpdateEvent

PaymentInstrument is a non-standard dictionary and hasn't its own documentation, so remove its mention

payerdetailchange event is fired on PaymentResponse rather then PaymentRequest

PaymentShippingOption is a non-standard dictionary and hasn't its own documentation

PaymentDetailsModifier is a dictionary and hasn't its own documentation, so add for its detail

AddressErrors is a non-standard dictionary and hasn't its own documentation

retry() and complete() methods are on PaymentResponse just removes these links

Motivation

Additional details

Related issues and pull requests

@github-actions github-actions bot added the Content:WebAPI Web API docs label Jan 5, 2024
@skyclouds2001 skyclouds2001 marked this pull request as ready for review January 5, 2024 13:38
@skyclouds2001 skyclouds2001 requested a review from a team as a code owner January 5, 2024 13:38
@skyclouds2001 skyclouds2001 requested review from Elchi3 and removed request for a team January 5, 2024 13:38
Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @skyclouds2001 !

It's important that we explain the syntax of dictionary objects, even if we don't have a separate page for them.

Also it looks like:

@teoli2003
Copy link
Contributor

Yes, this page is a complex one.

The general rule about dictionaries is that:

  • we don't mention the dictionary name because it is not visible to the user.
  • on each page where a dictionary is used, a list of its fields must be listed.

We do sometimes make exceptions:

  • for very long dictionaries used on several pages, we can have a specific entry (not the cases here)
  • when we have long dictionaries used in one constructor and one getter, we sometimes describe them in the constructor and only link to the object description of it in the getter. There must be a link to it.
  • we don't need to describe everything for each object occurrence on the page, but only once per page, usually in the Value or Return value sections or in the Parameters one.

The difficulty on this API is that some of these dictionaries are being phased out (non-standard or deprecated) but have not been removed from the implementation. This is a reason why this has not been cleaned before.

Copy link
Contributor

@teoli2003 teoli2003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two sentences were not clear to me (it wasn't an extensive review).

files/en-us/web/api/paymentrequest/show/index.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added the size/l [PR only] 501-1000 LoC changed label Jan 21, 2024
Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thank you!

@wbamberg wbamberg merged commit 5239b29 into mdn:main Jan 22, 2024
8 checks passed
@skyclouds2001 skyclouds2001 deleted the payment-links branch January 23, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/l [PR only] 501-1000 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants